Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed issues with resource token #13855

Merged
merged 13 commits into from
Sep 24, 2020
Merged

Fixed issues with resource token #13855

merged 13 commits into from
Sep 24, 2020

Conversation

srinathnarayanan
Copy link
Contributor

  • Earlier, the SDK was doing a comparison by id to check if the resource token was for a particular resource was passed to the client
  • Now, we check for the entire path of the resource to be a key to the resource token passed to the client
  • Changed existing tests to reflect this behavior.

@ghost ghost added the Cosmos label Sep 18, 2020
southpolesteve
southpolesteve previously approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@southpolesteve southpolesteve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add changelog

annatisch
annatisch reviewed Sep 21, 2020
View reviewed changes
sdk/cosmos/azure-cosmos/test/test_crud.py
resource_tokens[urllib.quote(all_collection.id)] = (all_permission.properties['_token'])
resource_tokens[urllib.quote(read_collection.id)] = (read_permission.properties['_token'])
resource_tokens["dbs/" + created_db.id + "/colls/" + all_collection.id] = (all_permission.properties['_token'])
resource_tokens["dbs/" + created_db.id + "/colls/" + read_collection.id] = (read_permission.properties['_token'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the existing test invalid in some way?
Rather than alter existing tests, we should simply add new ones - we don't want to inadvertently break existing customers. All the existing tests should pass without modification (unless the tests were never correct in the first place).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the way in which resource tokens were implemented were faulty. resource tokens are supposed to be a map of resource link to token . i.e {"dbs/mydb/coll/mycoll1": "token1", "dbs/mydb/colls/mycoll2": "token2"} (this is the implementation in the .Net SDK and is the spec as well). But in the python sdk currently, we expect to get it to be a map of resource id to token. i.e {"mycoll1": "token1", "mycoll2": "token2"}. This is faulty since the same collection name can exist in 2 different databases.
These tests also follow the old pattern. They will break with the new changes, so I changed the tests as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but is it possible any customers were using the old faulty pattern? For example by manually building this dictionary? Or if they had done that would it not have worked?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annatisch It is possible that some cases would have worked but that is not intentional. We consider it a bug and fixed the same issue in JS SDK recently.

Previous versions of the Cosmos SDK were all based on rid which was a unique identifier resource. The transition to user supplied ids a couple years ago changed that, but we didn't fix resource tokens to account for it. Given all this, and the fact that resource tokens do not see a lot of usage, I am fine with merging this as a bugfix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still prefer this to be backwards compatible - but if you've rolled out the same change in other languages I guess that's okay.

@Rodrigossz Rodrigossz mentioned this pull request Sep 21, 2020
Closed
@annatisch
Copy link
Member

/azp run python - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@srinathnarayanan srinathnarayanan merged commit 9f9a544 into master Sep 24, 2020
@srinathnarayanan srinathnarayanan deleted the users/srnara/resourceTokenFIx branch September 24, 2020 16:25
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Sep 29, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
1832a1e 
…into path_format_args_polling

* 'master' of https://github.com/Azure/azure-sdk-for-python: (46 commits)
  Fix typo in track 2 migration guide (Azure#14006)
  commit metrics advisor (Azure#14072)
  [eventgrid] Update README.md (Azure#14030)
  Update Key Vault readmes (Azure#14068)
  Update CODEOWNERS (Azure#14054)
  Correct azure-eventhub example (Azure#14050)
  Replace docstring code blocks with literalincludes (Azure#13948)
  Added more docstring context and sample code for `receive_messages()` (Azure#13789)
  Form 2.1-preview.1 (Azure#13770)
  Sync eng/common directory with azure-sdk-tools repository for Tools PR 1029 (Azure#13896)
  Service Mapping Update for Communication (Azure#13979)
  update sample to show facet+count & collection type (Azure#13909)
  Increment package version after release of azure_eventgrid (Azure#14020)
  Update CHANGELOG.md (Azure#14014)
  fixed delete blob types (Azure#14001)
  Fixed issues with resource token (Azure#13855)
  update samples and readme (Azure#13999)
  update dependent version of azure-core (Azure#13986)
  Sync eng/common directory with azure-sdk-tools repository for Tools PR 1041 (Azure#14005)
  Track2-azure-mgmt-baremetalinfrastructure-2020-9-22 (Azure#13941)
  ...
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request May 10, 2021
@VMMicrosoft @rewongmicrosoft
@brandong-ms @akning-ms
CodeGen from PR 14339 in Azure/azure-rest-api-specs
12362e6 
Release microsoft.sql 2021 02 01 preview (Azure#14339)

* Adds base for updating Microsoft.Sql from version preview/2020-11-01-preview to version 2021-02-01-preview

* Updates readme

* Updates API version in new specs and examples

* Fix Integer format: S360 swagger lint issues in 2021-02-01 (Azure#13855)

* Update integer format

* update typo

* Swagger Documentation for Outbound Firewall Rules (Azure#13820)

* Swagger Documentation for Outbound Firewall Rules

* Add the new file to v5

Co-authored-by: Vineet Mahadik <[email protected]>

* Swagger Documentation For Database Resource With Ledger (Azure#13916)

* adding database swagger files

* modifying example

* Fixing to be boolean

* adding database extensions and database operations

* adding other database examples

* removing database_legacy, adding usages

* removing usages

* removing databases_legacy from readme

* Swagger Documentation For Ledger Digest Upload (Azure#13871)

* adding ledger api

* Fixes from validation

* removing required endpoint parameter, adding ledgerName (current) to id

* removing 201 response from resource, renaming ledgerName

* adding location to 202 results

* fixing 202 response location

* renaming files

* updating files with new controller name + correct entity name

* fixing readme

* Dev brandong getrestorabledropped (Azure#14129)

* Update RestorableDroppedDatabases API to add BackupStorageAccountType property

* Update readme.md

* Re-add elasticPoolId as a deprecated property

* Update elasticPoolId description and remove trailing comma causing failures

* Remove the unsupported deprecated property

* Add 2021-02-01-preview minor changes (Azure#13942)

* add 2021-02-01-preview for test

* update with 2021-04-19 latest

* update readme.md

* update with latest master in DSMainDev

* Carry IsInfraEncryptionEnabled to Database.json (Azure#14322)

* carry latest minor changes.

* re-format readme.md

* remove 2020 11 01 RestorableDroppedManagedDatabases in V5

* Update readme file in 2021 02 01 dev branch to match the master branch (Azure#14336)

* Carry IsInfraEncryptionEnabled to Database.json (Azure#14322)

* carry latest minor changes.

* re-format readme.md

* remove 2020 11 01 RestorableDroppedManagedDatabases in V5

* update to match master branch readme.md

Co-authored-by: Vineet Mahadik <[email protected]>
Co-authored-by: Vineet Mahadik <[email protected]>
Co-authored-by: rewongmicrosoft <[email protected]>
Co-authored-by: brandong-ms <[email protected]>
Co-authored-by: Arthur Ning <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@southpolesteve southpolesteve southpolesteve left review comments

@annatisch annatisch annatisch approved these changes

@zfoster zfoster Awaiting requested review from zfoster

Assignees
No one assigned
Labels
Cosmos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@srinathnarayanan @annatisch @southpolesteve